Claude finished @phernandez's task in 1m 53s —— View job

PR Review: feat(cli): support cloud project visibility on add

Tasks:

• Read changed files and gather context
• Review code quality and standards
• Review testing coverage
• Review architecture and security
• Post final review

Summary

This PR is well-structured overall — the refactor of _resolve_workspace_id into a shared helper is a clear improvement that reduces duplication from set_cloud. The schema extension and new tests cover the happy paths. One notable bug and a few minor issues below.

Code Quality & Standards Checklist

Issues Found

🔴 Bug: workspace_id not persisted for cloud-only adds (no --local-path)

src/basic_memory/cli/commands/project.py:406

if effective_cloud_mode and local_sync_path:   # ← workspace_id only saved here
    ...
    if resolved_workspace_id:
        entry.workspace_id = resolved_workspace_id

The entire config-save block is guarded by local_sync_path being truthy. When a user runs:

bm project add research --cloud --workspace "Basic Memory"

...the workspace is resolved to a tenant_id, passed to the API correctly, but never written to config. The next get_client(workspace=...) call for this project would have no workspace routing context.

The fix should extract a separate block to persist workspace_id even when no --local-path is given:

# Always persist workspace_id when resolved, regardless of local sync path
if effective_cloud_mode and resolved_workspace_id:
    entry = config.projects.get(name)
    if entry:
        entry.workspace_id = resolved_workspace_id
    else:
        config.projects[name] = ProjectEntry(
            path="",
            workspace_id=resolved_workspace_id,
        )
    ConfigManager().save_config(config)

Fix this →

🟡 Duplicate import of get_available_workspaces in _resolve_workspace_id

src/basic_memory/cli/commands/project.py:66–100

The function imports get_available_workspaces twice — once in the if workspace is not None branch (alongside _workspace_choices and _workspace_matches_identifier) and again in the try fallback block. These can be collapsed into a single import at the top of the function to avoid the redundancy:

def _resolve_workspace_id(config, workspace: str | None) -> str | None:
    from basic_memory.mcp.project_context import (
        _workspace_choices,
        _workspace_matches_identifier,
        get_available_workspaces,
    )
    ...

🟡 _normalize_project_visibility return type should use ProjectVisibility

src/basic_memory/cli/commands/project.py:57

The function signature returns str but the ProjectVisibility type alias (Literal["workspace", "shared", "private"]) is already defined in cloud.py and imported by cloud_utils.py. Using it here would give the type checker stronger guarantees:

from basic_memory.schemas.cloud import ProjectVisibility

def _normalize_project_visibility(visibility: str | None) -> ProjectVisibility:

🟡 Missing test: workspace_id not persisted in cloud-only add (no --local-path) with --workspace

tests/cli/test_project_add_with_local_path.py

test_project_add_cloud_workspace_resolves_and_persists only exercises the --local-path path. There is no test verifying that --workspace without --local-path either correctly persists workspace_id or documents the expected behavior. This gap is related to the bug above.

ℹ️ Importing private helpers from project_context

src/basic_memory/cli/commands/project.py:70–72

from basic_memory.mcp.project_context import (
    _workspace_choices,
    _workspace_matches_identifier,
    ...
)

These functions are intentionally "private" (underscore-prefix). The same pattern already exists in set_cloud (now consolidated via _resolve_workspace_id), so this is fine for now — but consider whether these should be promoted to a public API or extracted to a shared workspace_utils module if reuse grows.

Schema Change

CloudProjectCreateRequest in cloud.py adds visibility with default="workspace" — this is backward-compatible for any existing call sites that don't pass visibility. ✅

Overall

The core logic is correct and the refactor is clean. The main issue to address before merging is the workspace_id not being saved for cloud-only adds. The tests covering workspace resolution + persistence only exercise the --local-path path and would miss this regression.

Claude finished @phernandez's task in 1m 30s —— View job

PR Review: feat(cli): support cloud project visibility on add

Tasks:

• Read changed files and gather context
• Check if previous review issues were addressed
• Review code quality and standards
• Review testing coverage
• Post final review

Summary

This update directly addresses the issues raised in the previous review. All three actionable items have been resolved cleanly.

Previous Review Issues — Status

Bug fix (project.py:408): The condition is now if effective_cloud_mode and (local_sync_path or resolved_workspace_id), correctly persisting workspace_id even when no local sync path is provided.

Import consolidation (project.py:75–79): All three helpers (_workspace_choices, _workspace_matches_identifier, get_available_workspaces) are now imported once at the top of _resolve_workspace_id, eliminating the duplicate get_available_workspaces import from the fallback branch.

Return type (project.py:58): _normalize_project_visibility now returns ProjectVisibility and uses cast() correctly.

New test (test_project_add_with_local_path.py:256–306): test_project_add_cloud_workspace_persists_without_local_path explicitly covers the previously untested code path and asserts entry["path"] == "", entry["mode"] == "cloud", entry["workspace_id"] == "...", and entry["local_sync_path"] is None.

Code Quality & Standards Checklist

Minor Observations (non-blocking)

_resolve_workspace_id missing type annotation on config parameter

src/basic_memory/cli/commands/project.py:71

def _resolve_workspace_id(config, workspace: str | None) -> str | None:

config is untyped. The codebase requires full type annotations (CLAUDE.md). The correct type would be BasicMemoryConfig (or whatever config_manager.load_config() returns). Not blocking, but worth a follow-up.

Schema Change

CloudProjectCreateRequest adds visibility: ProjectVisibility = "workspace" — backward-compatible with all existing call sites. ✅

Overall

The PR is ready to merge. All critical issues from the previous review have been addressed, test coverage is solid, and the refactor of _resolve_workspace_id into a shared helper is a net improvement over the previous duplication.